Optimize Visitor::visitInParallel - #1953
Open
OpaqueRock wants to merge 5 commits into
Open
Conversation
extractVisitFn() re-derives the same enter/leave callable via array lookups on every node, for every wrapped visitor - O(nodes x visitors) calls - even though its result for a given (visitor, kind, isLeaving) triple never changes during a traversal. Cache it lazily per visitor/kind on first use, using `false` as a "no callback" sentinel, so custom (non-NodeKind) Node kinds are also supported. Also replace func_get_args() plus argument unpacking in the enter/leave dispatcher closures with the five explicit, typed parameters they are always called with, avoiding func_get_args()'s per-call overhead.
OpaqueRock
added a commit
to OpaqueRock/graphql-php
that referenced
this pull request
Jul 22, 2026
The Visitor::visitInParallel() enter/leave callback caching optimization has been split out into a separate PR (webonyx#1953), so it can be reviewed independently of the Lexer::readString optimization here.
Contributor
There was a problem hiding this comment.
Pull request overview
Improves traversal performance in Visitor::visitInParallel() by avoiding repeated callback resolution and avoiding per-node func_get_args() overhead in the parallel visitor dispatch path.
Changes:
- Adds lazy per-(visitor index, node kind) caching for resolved
enter/leavecallbacks (withfalsesentinel for “no callback”). - Replaces argument unpacking via
func_get_args()with explicit dispatcher parameters to match theVisitor::visit()call site.
Comments suppressed due to low confidence (1)
src/Language/Visitor.php:452
- Same as
enter: makingleaverequire 5 parameters can break callers that invoke the dispatcher directly with only(Node $node). Consider defaulting the extra parameters to keep the callable usable in both 1-arg and 5-arg contexts.
'leave' => static function (Node $node, $key, $parent, array $path, array $ancestors) use ($visitors, $skipping, $visitorsCount, &$leaveFns) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/Language/Visitor.php:413
- The file-level
@phpstan-type NodeVisitordocuments visitor callbacks ascallable(Node), but this dispatcher now requires$key,$parent,$path, and$ancestors. Making these parameters optional keeps the documented callable(Node) contract (and avoidsArgumentCountErrorif invoked according to that contract) while still allowingVisitor::visit()to pass all 5 arguments.
'enter' => static function (Node $node, $key, $parent, array $path, array $ancestors) use ($visitors, $skipping, $visitorsCount, &$enterFns) {
src/Language/Visitor.php:452
- Same as
enter:NodeVisitoris documented ascallable(Node), but this dispatcher requires 5 parameters. Making the additional parameters optional keeps the documented visitor callable contract.
'leave' => static function (Node $node, $key, $parent, array $path, array $ancestors) use ($visitors, $skipping, $visitorsCount, &$leaveFns) {
src/Language/Visitor.php:456
$kind = $node->kinddoes not depend on the visitor index; hoisting it outside the loop avoids repeating the property read for each visitor on the leave path.
for ($i = 0; $i < $visitorsCount; ++$i) {
if ($skipping[$i] === null) {
$kind = $node->kind;
$fn = $leaveFns[$i][$kind] ?? null;
src/Language/Visitor.php:420
$kind = $node->kindis invariant for the whole dispatcher invocation, but it is currently re-read once per visitor. Hoist it outside the loop to avoid repeated property reads on this hot path.
This issue also appears on line 453 of the same file.
for ($i = 0; $i < $visitorsCount; ++$i) {
if ($skipping[$i] !== null) {
continue;
}
$kind = $node->kind;
$fn = $enterFns[$i][$kind] ?? null;
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out from #1948, so the
Visitoroptimization can be reviewed independently of theLexer::readStringoptimization (which remains in #1948).Problem
Visitor::visitInParallel()builds anenter/leavedispatcher closure that is invoked once per AST node. On every call, for every wrapped visitor, it calledextractVisitFn()to re-resolve the enter/leave callback for that visitor and the current node's kind — an O(nodes × visitors) number of array lookups. However, a callback's identity for a given(visitor, kind, isLeaving)triple never changes during a single traversal, so this work was being needlessly repeated for every node of the same kind.Additionally, the dispatcher closures used
func_get_args()plus argument unpacking ($fn(...func_get_args())) even though they are always invoked with the same five arguments, which carries avoidable per-call overhead.Change
NodeKind's fixed set), usingfalseas a "no callback" sentinel so??can distinguish "not yet cached" from "cached, nothing to call." Building the cache lazily also means visitors for custom (non-NodeKind) Node kinds are cached and dispatched correctly, same as before.func_get_args()/ argument unpacking in the enter/leave closures with the five explicit, typed parameters ($node, $key, $parent, array $path, array $ancestors) that matchVisitor::visit()'s single call site.No public API or behavior changes; this is purely an internal performance optimization of
visitInParallel()'s dispatch path.Testing
composer test— full suite passes.composer stan— no new errors.composer rector/composer php-cs-fixer— no changes required.